-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
R4R: Fix Non-Deterministic Test Coverage #1850
Conversation
99de729
to
2f93542
Compare
2f93542
to
56542da
Compare
Codecov Report
@@ Coverage Diff @@
## develop #1850 +/- ##
===========================================
+ Coverage 63.5% 63.52% +0.02%
===========================================
Files 118 118
Lines 7006 7016 +10
===========================================
+ Hits 4449 4457 +8
- Misses 2301 2302 +1
- Partials 256 257 +1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK! Good catch :).
While not super important for small PR's, things are much easier / faster to review if we separate out functionality changes from refactors. (i.e. renaming getkeybase)
Does this close #1639 as well? |
Fair enough, but sometimes I just cannot help myself 😆 , although I will try to limit it to just documentation improvements.
Not exactly. However, I have not seen that error in builds that I've looked at (which was a lot). I think it might be safe to close #1639 for now. |
56542da
to
6e92261
Compare
var ( | ||
err error | ||
res *http.Response | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol I don't like this (fine) but I may switch back at some point - why use 4 lines when you could use 2! - stylistically I'd think it's cleaner to only use var blocks for global variables - or under certain very specific situations where there a whole bunch of variables in a in a function (maybe 5+) - yeah the extra indentation is a bit confusing because in funcs typically means your in an If/switch/for
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, I suppose we have stylistic differences. I see the above as much cleaner and easier to read. Yes, granted, it's two extra lines, but Go is naturally a verbose language. However, I will limit such refactors in future work.
} | ||
|
||
// start the LCD. note this blocks! | ||
// startLCD starts the LCD. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a sentence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch - next time seperate out functionality changes from cleanup/refactors. Thanks! (Also see my comments) - still want to merge though
Took a look at about 15-20 pages worth of CircleCI builds and looked at the ones that failed due to
test_cover
. From what I gathered tests fail due to missing but expected Tendermint WAL files OR an address already being bound in LCD tests. Dug through the code and found that the only address that does not get modified on each LCD test is the GRPC address, coincidentally that is what I see in the logs:listen tcp 0.0.0.0:36658: bind: address already in use
.GetConfig
Note: The WAL issue will still need to be addressed, but I'm pretty sure that is TM side?
closes: #1675
docs/
)PENDING.md
that include links to the relevant issue or PR that most accurately describes the change.cmd/gaia
andexamples/
For Admin Use: